Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update notification for removing component #2016

Merged
merged 1 commit into from
Sep 27, 2019
Merged

Update notification for removing component #2016

merged 1 commit into from
Sep 27, 2019

Conversation

BeniCheni
Copy link
Contributor

@BeniCheni BeniCheni commented Sep 25, 2019 •
edited
Loading

Fixes #1993. (Previously reviewed in #2012 and corrected based on comment in this new PR)

Local Test
→ rustup update               
info: syncing channel updates for 'stable-x86_64-apple-darwin'
info: syncing channel updates for 'nightly-x86_64-apple-darwin'
info: latest update on 2019-09-25, rust version 1.39.0-nightly (6ef275e6c 2019-09-24)
info: downloading component 'clippy'
info: downloading component 'rustfmt'
info: downloading component 'rustc'
 60.1 MiB /  60.1 MiB (100 %)   7.6 MiB/s in  7s ETA:  0s
info: downloading component 'rust-std'
172.7 MiB / 172.7 MiB (100 %)   7.9 MiB/s in 22s ETA:  0s
info: downloading component 'cargo'
info: downloading component 'rust-docs'
 11.8 MiB /  11.8 MiB (100 %)   7.6 MiB/s in  1s ETA:  0s
info: downloading component 'rust-src'
info: removing previous version of component 'clippy'
info: removing previous version of component 'rustfmt'
info: removing previous version of component 'rustc'
info: removing previous version of component 'rust-std'
info: removing previous version of component 'cargo'
info: removing previous version of component 'rust-docs'
info: removing previous version of component 'rust-src'
info: installing component 'clippy'
info: installing component 'rustfmt'
info: installing component 'rustc'
 60.1 MiB /  60.1 MiB (100 %)   7.6 MiB/s in  8s ETA:  0s
info: installing component 'rust-std'
172.7 MiB / 172.7 MiB (100 %)  15.2 MiB/s in 10s ETA:  0s
info: installing component 'cargo'
info: installing component 'rust-docs'
 11.8 MiB /  11.8 MiB (100 %)   2.3 MiB/s in  4s ETA:  0s
info: installing component 'rust-src'
info: checking for self-updates

  stable-x86_64-apple-darwin unchanged - rustc 1.37.0 (eae3437df 2019-08-13)
   nightly-x86_64-apple-darwin updated - rustc 1.39.0-nightly (6ef275e6c 2019-09-24)

@BeniCheni BeniCheni mentioned this pull request Sep 25, 2019
Closed
@BeniCheni BeniCheni force-pushed the update-notification-removing-component branch from 6e7e52f to aaad735 Compare September 26, 2019 03:01
@BeniCheni
Copy link
Contributor Author

@thedrow, thanks for explaining. aaad735 updated to use a different approach w. update_from_dist param.

@BeniCheni BeniCheni force-pushed the update-notification-removing-component branch from aaad735 to 5d2b43d Compare September 26, 2019 21:01
@BeniCheni
Copy link
Contributor Author

BeniCheni commented Sep 26, 2019 •
edited
Loading

Tiny note that 5d2b43d simply rename to explicit_modify param, which seems more explicit than previous update_from_dist.

kinnison
kinnison suggested changes Sep 27, 2019
View reviewed changes
Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi!

Thanks for your submission. Just a rename of a variable (or some boolean sense inversions) to do and this looks mergeable from there!

D.

src/dist/manifestation.rs Outdated Show resolved Hide resolved
@BeniCheni
Update notification for removing component
1844a92 
Use update_from_dist flag for Notification::RemovingOldComponent

Rename explicit_modify param in update fn of dist manifestitation

Rename implicit_modify param in update fn of dist manifestitation
@BeniCheni BeniCheni force-pushed the update-notification-removing-component branch from 5d2b43d to 1844a92 Compare September 27, 2019 15:05
@BeniCheni
Copy link
Contributor Author

Alternatively call it implicit_modify and it'll be good as-is :D

Thank you, @kinnison. implicity_modify indeed sounds fit, and less risk not having to change the logic inversion in calling code & break something accidentally.

1844a92 updated to use implicity_modify.

kinnison
kinnison approved these changes Sep 27, 2019
View reviewed changes
Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me. 👍

Thank you, I'll just wait for the CI

@kinnison kinnison merged commit 70f8357 into rust-lang:master Sep 27, 2019
@BeniCheni BeniCheni deleted the update-notification-removing-component branch September 27, 2019 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kinnison kinnison kinnison approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@BeniCheni @kinnison